Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RAC] [RBAC] force rule data client to register features #20

Merged

Conversation

dhurley14
Copy link
Owner

Summary

force any plugin registering rules to register the feature id into the master list which maps feature id to index names

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@dhurley14 dhurley14 force-pushed the master_list_alerts_indices branch from a5d5ff5 to 879c0c0 Compare June 30, 2021 23:53
@@ -46,6 +46,15 @@ interface GetAlertParams {
index?: string;
}

export const validFeatureIds = ['apm', 'observability', 'siem'] as const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we distinguish between observability and apm, etc. Is apm a subset of observability?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data grid should know where it is.. but yeah I think that's what observability is going for with their "top alerts" rule which looked to be querying .alerts..

https://github.com/elastic/kibana/blob/5d95e2e0cd59438e76e7a585eed7ac382f78b057/x-pack/plugins/observability/server/lib/rules/get_top_alerts.ts#L27

Copy link

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few TS suggestions and a suggestion for where to store the main mapping object. Thanks so much!

alias: plugins.ruleRegistry.ruleDataService.getFullAssetName(),
feature: 'observability',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does listing the feature do here?

Copy link
Owner Author

@dhurley14 dhurley14 Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the feature param is to force the user to update the data structure which contains the mapping of consumers to alerts as data indices. The idea is it is typed such that it forces the user to go to the code and modify it. At least until a better system is put in place or we move the alerts as data client out of rule registry.

https://github.com/dhurley14/kibana/pull/20/files#diff-6ce4064181abc41103593d43b51edf466af5c7b67f98b5bcd184aa225e9b1386R41-R52

@dhurley14 dhurley14 force-pushed the master_list_alerts_indices branch from f213f64 to fbd3905 Compare July 1, 2021 20:13
@dhurley14 dhurley14 force-pushed the master_list_alerts_indices branch from c4b7ace to d2173f5 Compare July 1, 2021 20:29
Copy link

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and by G I mean GREAT

@dhurley14 dhurley14 merged this pull request into squashed_alerts_rbac_mvp_backup Jul 1, 2021
@dhurley14 dhurley14 deleted the master_list_alerts_indices branch July 1, 2021 21:08
yctercero pushed a commit that referenced this pull request Jul 5, 2021
* force any plugin registering rules to register the feature id into the master list which maps feature id to index names

* javascript is not python

* favor keyof instead of typeof as const, removes ts-expect-error

* updates typedocs

* adds rbac utils to utils folder and updates typedocs

* featureIds is already string[] and do not trust input for type guard
dhurley14 added a commit that referenced this pull request Jul 7, 2021
* force any plugin registering rules to register the feature id into the master list which maps feature id to index names

* javascript is not python

* favor keyof instead of typeof as const, removes ts-expect-error

* updates typedocs

* adds rbac utils to utils folder and updates typedocs

* featureIds is already string[] and do not trust input for type guard
dhurley14 added a commit that referenced this pull request Jul 8, 2021
* force any plugin registering rules to register the feature id into the master list which maps feature id to index names

* javascript is not python

* favor keyof instead of typeof as const, removes ts-expect-error

* updates typedocs

* adds rbac utils to utils folder and updates typedocs

* featureIds is already string[] and do not trust input for type guard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants